Fix boolean buffer overflow#191
Conversation
Signed-off-by: Richard Moulton <richard@rmsoftwareservices.co.uk>
Signed-off-by: Richard Moulton <richard@rmsoftwareservices.co.uk>
Signed-off-by: Richard Moulton <richard@rmsoftwareservices.co.uk>
218fd7c to
c4419c3
Compare
| } | ||
| break; | ||
| default: // SQL_CHAR / SQL_VARCHAR | ||
| default: // SQL_CHAR / SQL_VARCHAR / SQL_BOOLEAN and other string-representable types |
There was a problem hiding this comment.
TBH I think we should bind booleans as a boolean type not as a string anyway. As mentioned in the PR description, the value of SQL_DATALINK was changed in 7.5 so that SQL_BOOLEAN could use its value for consistency with Db2 LUW. I argued against this change, but it was determined that there would be minimal impact because basically nobody uses DATALINKs anyway so we may as well do the same.
- add code define SQL_BOOLEAN (and redefine SQL_DATALINK) if not defined
- adjust db2a.js to set SQL_DATALINK to -400.
- add SQL_BOOLEAN to db2a.js with value 16
There was a problem hiding this comment.
@kadler, apologies for the delay, I'd turned off notifications some time ago and had forgotten that fact when I raised this PR. I'll have a look at the changes you suggest.
There was a problem hiding this comment.
@kadler, I've just implemented the changes to correctly handle the boolean data type.
The SQL type code 16 has different meanings across IBM i releases:
- V7R4 and earlier: type 16 = DATALINK (no BOOLEAN type exists)
- V7R5 and later: type 16 = BOOLEAN, type -400 = DATALINK
Additionally, boolean parameter binding in master was broken - it used
SQL_C_BIT which IBM i CLI rejects with error HY003.
I've made three key changes in this PR:
- Normalize Type Constants (dberror.h)
- Fix Boolean Parameter Binding (dbstmt.cc)
- Improve String Column Handling (dbstmt.cc)
Backward Compatibility
✅ On V7R4: DATALINK columns (type 16) correctly returned as strings
✅ On V7R5+: BOOLEAN columns (type 16) correctly returned as booleans
✅ On V7R5+: DATALINK columns (type -400) correctly returned as strings
When running the tests on V7R4 (or lower) there will be four failures, the new boolean tests; the new datalink test will succeed. When running the tests on V7R5 (or higher) all test are successful.
Note that the datalink data type is not well supported by the connector. I have added tests that check for a datalink column in the result but work would be required to handle parameter binding.
I've not added any comments to README.md about changes to the BOOLEAN data type handling. Anybody already using the connector and BOOLEAN will now be dealing with a true boolean rather than a string representation. Happy to do that if you think it's appropriate.
Normalizes SQL type code 16 handling to support both V7R4 (DATALINK) and V7R5+ (BOOLEAN) by routing through string processing. Fixes broken boolean parameter binding (SQL_C_BIT → SQL_C_CHAR). Adds comprehensive tests for BOOLEAN and DATALINK data types. Signed-off-by: Richard Moulton <richard@rmsoftwareservices.co.uk>
Summary
Fix a buffer overread in the native addon that caused garbage characters to be appended to string values returned from queries. The bug is most easily reproduced with BOOLEAN columns, where querying a
FALSEvalue returns"FALSEdנ"or similar corrupted data.Root cause
Three issues contribute to the bug:
Undersized bind buffer for BOOLEAN —
bindColData()usescolPrecise * 4 + 1to size the bind buffer. BOOLEAN hascolPrecise=1(it's logically a 1-bit value), giving a 5-byte buffer. The DB2 CLI converts BOOLEAN to its string representation "FALSE" (5 chars), which fills the buffer with no room for a null terminator.Missing null terminator in result copy —
fetchData()allocatescalloc(colLen, ...)for the result buffer and copiescolLenbytes viamemcpy, with no extra byte for null termination.Null-terminated string creation —
buildJsObject()passes the result buffer toNapi::String::New(env, data), which reads until\0. Without a null terminator, it reads past the buffer into heap memory, producing garbage characters.Changes
bindColData()— enforce a minimum bind buffer size of 6 bytes in thedefaultcase, ensuring BOOLEAN's "FALSE" representation (5 chars + null) always fitsfetchData()— allocatecolLen + 1bytes for the result buffer, ensuring null termination aftermemcpybuildJsObject()— use the length-awareNapi::String::New(env, data, rlength)overload as defense in depth, so string creation is bounded by the known data length rather than relying on null terminationdberror.h— updateSQL_DATALINKcomment to note its value changed from 16 to -400 in IBM i 7.5 (value 16 is nowSQL_BOOLEAN)Observations
sqlcli-develyum package does not yet include aSQL_BOOLEANdefinition, even though the system headerQSYSINC/H(SQLCLI)has been updated in IBM i 7.5Test plan